Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Chore: Remove v1 fields and reconcile logic from dspo #712

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Chore: Remove v1 fields and reconcile logic from dspo #712

wants to merge 2 commits into from

Conversation

HumairAK
Copy link
Contributor

@HumairAK HumairAK commented Oct 1, 2024 •
edited
Loading

The issue resolved by this Pull Request:

Resolves #https://issues.redhat.com/browse/RHOAIENG-13572

Description of your changes:

This PR removes V1 from DSPO, more specifically it:

  • Removes all v1 only fields from DSPA CR
  • DSPA version field here, is assumed as "v2" by default when not provided (previously it was v1)
    • If user specifies v1, DSPO throws a reconcile log message and does not proceed with the reconcilliation for that
    • If user specifies v1, DSPA status reports a meaningful error with suggested action
  • DSPO continues to reconcile on the v1alpha1 resources (for both v1/v2), but when it encounters v1 version set in .spec.dspversion field in DSPA, dspo logs an error and dspa status error condition is thrown
  • DSPA CRD continues to serve at v1apha1, but has a deprecation set with a warning
  • DSPA uses a conversion strategy of None (default) 1. In this scenario when fetching the v1alpha1 resources, they are served with the unknown v1 fields as pruned.
  • Removes tekton related RBAC grants to pipeline runner SA, as well as the same rbac requirements on the DSPO
  • Removes any logic that will reconcile on v1 related fields
  • Updates all unit tests and functional tests to only test for v2, and remove any v1 related tests
  • DSPO should not trigger reconcilliations on custom watches on resources based on labels that belong to v1, example this

Testing instructions

DSPA v2 before after upgrade diff, only showing the relevant bits, the rest is the same
image

When v1 is detected, DSPO will log:

2024-10-01T20:06:08Z INFO unsupported DSP version v1 detected. Please manually remove this DSP resource and re-apply with a supported version field set {"namespace": "dspa1", "dspa_name": "sample"}

and show the following dspa status:

image

otherwise the DSPA v1 cr is left untouched, only the removed deprecated fields are synch'd by the cluster.

DSPO logs should not be showing any stacktrace errors.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Oct 1, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from humairak. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@HumairAK
Copy link
Contributor Author

HumairAK commented Oct 1, 2024

/hold

@dsp-developers
Copy link
Contributor

A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712
An OCP cluster where you are logged in as cluster admin is required.

To use this image run the following:

cd $(mktemp -d)
git clone git@github.com:opendatahub-io/data-science-pipelines-operator.git
cd data-science-pipelines-operator/
git fetch origin pull/712/head
git checkout -b pullrequest 064a1e6d7334dab3ad37937c29885cdd42de39d8
oc new-project opendatahub
make deploy IMG="quay.io/opendatahub/data-science-pipelines-operator:pr-712"

More instructions here on how to deploy and test a Data Science Pipelines Application.

@gregsheremeta
Copy link
Contributor

I think a fine way to test this is to just make sure v2 still works 🤷

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

@HumairAK HumairAK changed the title Chore: Remove v1 fields and reconcile logic from dspo WIP: Chore: Remove v1 fields and reconcile logic from dspo Oct 1, 2024
@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

jgarciao
jgarciao reviewed Oct 2, 2024
View reviewed changes
api/v1/groupversion_info.go Outdated Show resolved Hide resolved
api/v1/groupversion_info.go Show resolved Hide resolved
@HumairAK
Copy link
Contributor Author

HumairAK commented Oct 2, 2024 •
edited
Loading

FYI this is likely going to go through many significant changes, I would hold off on reviews for now

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

@HumairAK HumairAK changed the title WIP: Chore: Remove v1 fields and reconcile logic from dspo Chore: Remove v1 fields and reconcile logic from dspo Oct 3, 2024
@HumairAK
Copy link
Contributor Author

HumairAK commented Oct 3, 2024

/retest

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

1 similar comment
@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

@HumairAK
remove dsp v1
cd6d5a6 
This change adds a new apiversion "v1", which will remove all the dsp "v1" related fields, and also defaults to dsp "v2". The new apiversion is set as the stored version, however k8s will continue to serve v1alpha1, however it is deprecated.

All tests are updated acordingly.

Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com>
@HumairAK
Set dsp-version label on managed resources
aaf3510 
This change makes it so that new reconciles on dspa owned resources are
labelled with a dsp-version=<dspa-version> label. This is to allow for
the dspa selectively watch (for manual WatchesRawSources) on resources
that are descendents of DSPAs with .spec.dspVersion set to supported
versions.

Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com>
@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgarciao jgarciao jgarciao left review comments

@rimolive rimolive Awaiting requested review from rimolive

@VaniHaripriya VaniHaripriya Awaiting requested review from VaniHaripriya

Assignees
No one assigned
Labels
do-not-merge/hold
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@HumairAK @dsp-developers @gregsheremeta @jgarciao @openshift-merge-robot